Skip to content

fix(test): isolate soak harness daemon runtime - #2196

Merged
DeusData merged 1 commit into
DeusData:mainfrom
astandrik:fix/1696-soak-runtime-isolation
Sep 20, 2026
Merged

fix(test): isolate soak harness daemon runtime#2196
DeusData merged 1 commit into
DeusData:mainfrom
astandrik:fix/1696-soak-runtime-isolation

Conversation

@astandrik

@astandrik astandrik commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Part of #1696 (audit ledger), follow-up to #1691/#1695.

scripts/soak-test.sh claimed to "isolate daemon coordination from interactive CBM
sessions" through a private CBM_CACHE_DIR, but only CBM_RUNTIME_DIR moves the
daemon rendezvous. The soak must be the only client of the daemon it measures (it
asserts that a session crash and the final shutdown each stop the daemon), so on a
shared rendezvous it either stopped the operator's daemon or was refused with a
cache-root conflict.

  • scripts/soak-test.sh sources scripts/test-runtime.sh and calls
    cbm_test_runtime_init; off native Windows the helper's owner-only cache is the
    soak cache, so the daemon log path is unchanged in shape. soak_cleanup now runs
    cbm_test_runtime_cleanup "$BINARY" instead of rm -rf on the cache: the private
    daemon is stopped first and the root is kept when it will not stop. The
    native-Windows stamped root and its icacls handling are untouched
    (tests/test_soak_daemon_recovery_contract.sh pins them). Two FAIL messages no
    longer say "shared daemon".
  • tests/test_soak_runtime_isolation_contract.sh: environment-probe fixture,
    same assertions as the smoke contract, RESULTS_DIR kept out of the checkout.
    Fails on main with FAIL: soak-test exposed the caller CBM_RUNTIME_DIR to a product process; passes with this change. Wired as Step 0d2 in scripts/test.sh.

scripts/soak-legs.sh starts no product process and needs no change.

Checklist

  • Every commit is signed off (git commit -s)
  • Tests pass locally (make -f Makefile.cbm test) — shell-only change; the new
    contract plus test_soak_daemon_recovery_contract, test_venue_parity_contract
    and test_runtime_isolation_contract were run; the full C suite was not run
    on this host
  • Lint passes (make -f Makefile.cbm lint-ci) — no C changes
  • New behavior is covered by a test (reproduce-first for bug fixes)

@astandrik
fix(test): isolate soak harness daemon runtime
81a8a03 
scripts/soak-test.sh claimed to isolate daemon coordination from
interactive sessions through a private CBM_CACHE_DIR, but only
CBM_RUNTIME_DIR moves the daemon rendezvous, so the soak shared the
operator's account daemon and asserted that its own shutdown stopped it.
Source scripts/test-runtime.sh for a harness-owned runtime and cache,
and let the helper stop the private daemon before the root is removed.

Add tests/test_soak_runtime_isolation_contract.sh, which fails before
this change, and wire it into scripts/test.sh.

Part of DeusData#1696.

Signed-off-by: Anton Standrik <astandrik@yandex-team.ru>
@astandrik
astandrik requested a review from DeusData as a code owner September 13, 2026 10:52
Copilot AI lite review requested due to automatic review settings September 13, 2026 10:52
@github-actions

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@astandrik astandrik mentioned this pull request Sep 13, 2026
Open
Copilot AI reviewed Sep 13, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Address the native-Windows cleanup and setup-failure root-leak issues.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR isolates the soak-test daemon runtime and cache from interactive sessions and adds regression coverage.

Changes:

  • Uses private runtime and cache directories.
  • Adds a runtime-isolation contract test.
  • Wires the contract into scripts/test.sh.
File summaries
File Summary
tests/test_soak_runtime_isolation_contract.sh Verifies caller runtime and cache paths are not exposed.
scripts/test.sh Runs the new isolation contract.
scripts/soak-test.sh Initializes and cleans up the private runtime; two moderate cleanup issues remain on native Windows.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/soak-test.sh
# shellcheck source=test-runtime.sh
source "$(dirname "${BASH_SOURCE[0]}")/test-runtime.sh"
cbm_test_runtime_init
trap 'cbm_test_runtime_cleanup "$BINARY"' EXIT
Comment thread scripts/soak-test.sh
# native-Windows cache sits under SOAK_WIN_ROOT, which goes only after the
# daemon check because the binary it probes with is the copy inside it.
cbm_test_runtime_cleanup "$BINARY"
[ -z "${SOAK_WIN_ROOT:-}" ] || rm -rf -- "$SOAK_WIN_ROOT"
@DeusData

DeusData commented Sep 20, 2026

Copy link
Copy Markdown
Owner

Merged — thank you. This is the fix for a problem that has been quietly taxing other people's pull requests for weeks, and you went after the actual cause rather than the symptom.

The diagnosis in your comment is exactly right, and it matches what we had already verified from the other end: cbm_daemon_rendezvous_key() hashes a compile-time constant, so the daemon cohort is machine-global, and CBM_RUNTIME_DIR is the only hook that moves it. A private CBM_CACHE_DIR looks like isolation and is not — which is why the soak either stopped the operator's daemon or was refused with a cache-root conflict.

Two things I want to call out specifically:

Your contract test can fail. I checked rather than trusting the green — reverting scripts/soak-test.sh to its previous state while leaving your test in place:

FAIL: soak-test exposed the caller CBM_RUNTIME_DIR to a product process
(exit 1)

Driving the harness with an environment-probe fixture and asserting on what the product process actually receives is the right design. It tests the contract, not the implementation, so it will keep working when the harness changes again.

The cleanup is careful. Routing through cbm_test_runtime_cleanup means you inherit its refusals — unexpected basename, symlinked root, replaced runtime path — and it is idempotent, so the early trap ... EXIT and the later soak_cleanup both firing is harmless. Leaving the root behind when the daemon will not stop is the right call for diagnosis, and ordering the SOAK_WIN_ROOT removal after the daemon check because the probing binary lives inside it is a detail that would have bitten later.

One note for your planning: the rest of this series (#2197, #2198, #2199) lands on scripts/test.sh too, so expect to rebase the remaining ones as each lands. I will keep them moving.

@DeusData
DeusData merged commit 6cd1900 into DeusData:main Sep 20, 2026
33 checks passed
This was referenced Sep 20, 2026
Merged
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@DeusData DeusData Awaiting requested review from DeusData DeusData is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@astandrik @DeusData